-
Notifications
You must be signed in to change notification settings - Fork 191
stdlib_system
: essential path functionality
#999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR, @wassup05. Just a minor point (not related to functionality) about examples for now: It's always useful - esp. for those learning by examples - to have an idea of the expected output. Other stdlib examples add the expected print results as a comment below the respective commands. I would recommend to do the same here. |
I will add that with the other reviews @sebastian-mutz, I don't want to trigger the ci/cd workflow just for a few comments. Also on a second thought maybe instead of the |
The issue with Ninja detecting a "circular dependency" (there was non) was related to the stdlib_system.F90 file not being visible in a correct manner in the CMakeLists.txt file. The group: # Preprocessed files to contain preprocessor directives -> .F90
set(cppFiles corresponds to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @wassup05! Thanks for this nice work!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces essential path manipulation functions and related operator overloads to simplify path handling across platforms. Key changes include:
- Implementation of joinpath (and its operator(/)) for concatenating paths.
- Addition of splitpath and its derivatives (basename, dirname) for decomposing paths.
- Updates to tests, examples, documentation, and build files to support and illustrate the new functionality.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
test/system/test_path.f90 | Adds unit tests for new path functions and operator functionality. |
test/system/CMakeLists.txt | Registers the new "path" test. |
src/stdlib_system_path.f90 | Implements joinpath, operator(/), splitpath, basename, and dirname. |
src/stdlib_system.F90 | Defines interfaces and exports for the new path functionality. |
src/CMakeLists.txt | Includes the new path source file in the build. |
example/system/*.f90 | Provides usage examples for joinpath, splitpath, dirname, and basename. |
doc/specs/stdlib_system.md | Updates the spec documentation to incorporate the new interfaces. |
config/fypp_deployment.py | Passes the uppercase system name flag to the build commands. |
CMakeLists.txt | Adds compile options to define the system name macro. |
Comments suppressed due to low confidence (2)
example/system/example_path_dirname.f90:2
- The program name 'example_path_splitpath' does not match the file's purpose (dirname functionality). Consider renaming it to 'example_path_dirname'.
program example_path_splitpath
example/system/example_path_basename.f90:2
- The program name 'example_path_splitpath' in this file appears to be a copy-paste error; it should reflect basename functionality (e.g., 'example_path_basename').
program example_path_splitpath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start @wassup05, thank you for this PR. It's a bit long so I tried to give some initial comments first hand. Let's first discuss and iterate around these comments, and then we can start planning about more path functionality, thank you.
@@ -31,6 +31,12 @@ if(CMAKE_Fortran_COMPILER_ID STREQUAL GNU AND CMAKE_Fortran_COMPILER_VERSION VER | |||
message(FATAL_ERROR "GCC Version 9 or newer required") | |||
endif() | |||
|
|||
# Convert CMAKE_SYSTEM_NAME to uppercase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not currently possible to pass system-specific pre-processor flags to an fpm
build of stdlib
. So, I suggest CMake-determined macros should be removed.
@@ -115,6 +116,7 @@ def fpm_build(args,unknown): | |||
for idx, arg in enumerate(unknown): | |||
if arg.startswith("--flag"): | |||
flags= flags + unknown[idx+1] | |||
flags = flags + "-D{}".format(platform.system().upper()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System-dependent pre-processor flags cannot be passed to the stdlib-fpm-*
branches.
@@ -532,3 +532,175 @@ The file is removed from the filesystem if the operation is successful. If the o | |||
```fortran | |||
{!example/system/example_delete_file.f90!} | |||
``` | |||
|
|||
## `joinpath` - Joins the provided paths according to the OS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting #939 (review), I agree it would be a good idea to keep the naming convention as homogeneous as possible. In this case it would be join_path
with snake case.
@@ -83,7 +83,22 @@ module stdlib_system | |||
public :: kill | |||
public :: elapsed | |||
public :: is_windows | |||
|
|||
|
|||
!! Public path related functions and interfaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the said limitations, I would discourage to define these parameter
s with pre-processor flags. I would suggest the same approach for OS_TYPE be also adopted here.
logical, parameter, public :: ISWIN = .true. | ||
#else | ||
character(len=1), parameter, public :: pathsep = '/' | ||
logical, parameter, public :: ISWIN = .false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISWIN
looks like a duplicate definition for OS_TYPE()==OS_WINDOWS
, so I suggest to remove it altogether and use what stdlib
already offers at this stage. pathsep
or rather path_sep
would become a function: how about just sep
such as in Matlab? Or, something familiar to other libraries would be fine imho.
public :: joinpath | ||
public :: operator(/) | ||
public :: splitpath | ||
public :: basename | ||
public :: dirname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add short FORD comment blocks for theese APIs? As an example:
Lines 102 to 116 in ca867f8
!! version: experimental | |
!! | |
!! Tests if a given path matches an existing directory. | |
!! ([Specification](../page/specs/stdlib_system.html#is_directory-test-if-a-path-is-a-directory)) | |
!! | |
!!### Summary | |
!! Function to evaluate whether a specified path corresponds to an existing directory. | |
!! | |
!!### Description | |
!! | |
!! This function checks if a given file system path is a directory. It is cross-platform and utilizes | |
!! native system calls. It supports common operating systems such as Linux, macOS, | |
!! Windows, and various UNIX-like environments. On unsupported operating systems, the function will return `.false.`. | |
!! | |
public :: is_directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the documentation is added here, it should be placed following the interface
declaration or within the function
declaration if there is no additional interface, and not before the public
statement as it is done within this module currently. I actually have issues with FORD locally to produce the documentation because of that but haven't mention it yet to avoid mixing issues within the same PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a good idea to address (these and perhaps more of) FORD issues in a separate PR. For the current PR, I think it's ok to stay consistent with the current syntax.
paths = [character(20) :: 'C:','Users','Bob','Pictures','2025'] | ||
path = joinpath(paths) | ||
|
||
call checkpath(error, 'joinpath', 'C:\Users\Bob\Pictures\2025', path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the usual issues with MS Windows is paths with spaces. What will happen if we try to join "C:\Users\John Doe"
(note the quotes) + Pictures
+ 2025
?
end function join_op | ||
end interface operator(/) | ||
|
||
interface splitpath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to rename this to split_path
for stdlib style guide.
end subroutine splitpath | ||
end interface splitpath | ||
|
||
interface basename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to rename this to base_name
for consistency with the style guide.
end function basename | ||
end interface basename | ||
|
||
interface dirname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to rename this to dir_name
for consistency with the style guide.
A few path related functions for ease of future functionality have been added.
joinpath
: joins the given paths according to the platform'spath-separator
.operator(/)
: as was suggested in the Fortran discourse here an operator is also provided for the same functionalitysplitpath
: splits the path following the lastpath-separator
and returns thehead
andtail
.basename
: just returns thetail
ofsplitpath
dirname
: just returns thehead
ofsplitpath
The
pathsep
parameter
contains either/
or\
depending on the platform and is a compile-time constant now, so is theparameter
,ISWIN
Do let me know your thoughts.